-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
usdShade: Adding NormalMapTexture validator #3359
base: dev
Are you sure you want to change the base?
usdShade: Adding NormalMapTexture validator #3359
Conversation
{ | ||
errors.emplace_back( | ||
UsdShadeValidationErrorNameTokens->nonCompliantScale, | ||
UsdValidationErrorType::Warn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tallytalwar in the compliance checker, this is a warning, however the bias check below is an error. Is that correct? I wanted to change the bias error below to be a warning, let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referring the original commit for bias and scale adjustments to compliance checker: b718721
When complying with UsdPreviewTexture and UsdUVTexture (that is when nonCompliantScaleValues is false) we need to make sure to report error for bias values. Refer these here: https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usdImaging/plugin/usdShaders/shaders/shaderDefs.usda#L100-L112
Regarding non-conforming scale values being reported as warnings: There is a reasoning here: https://github.com/PixarAnimationStudios/OpenUSD/blob/dev/pxr/usd/usdUtils/complianceChecker.py#L550-L552 (I will request to keep all these code comments as-is in the cpp so as to save the reasoning). I believe Blender is the application mentioned here (cc. @dgovil is that still the case?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made sure to include all comments from the complianceChecker code (I did miss one). Also rebased this branch
} | ||
|
||
void | ||
TestUsdShadeNormalMapTextureValidator() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tallytalwar There are quite a few failure conditions in this test and validator, let me know if you think it would be better to break this up in to multiple validators or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tallytalwar We had the issue in the past with the encapsulation validator where me adding external files did not work for you, just calling this out here as this may happen again as I don't know what the difference was here in the past.
Filed as internal issue #USD-10296 |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
- added new validatorTokens - added implementation to match previous complianceChecker implementation - added tests to test each failure condition
ad1424b
to
454ae8c
Compare
- remove unnecessary check - add comment from complianceChecker.py that was missed
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
Description of Change(s)
Added an implementation of NormalMapTexture validation rules
Fixes Issue(s)